Update the global audio player #3265
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openverse-bot
added
the
🚦 status: awaiting triage
github-actions
bot
added
🧱 stack: documentation
|
Size Change: +2.75 kB (0%) Total Size: 949 kB
ℹ️ View Unchanged
|
|
Full-stack documentation: https://docs.openverse.org/_preview/3265 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
obulat
added
🟨 priority: medium
fcoveram
requested changes
Oct 30, 2023
obulat
commented
Oct 30, 2023
frontend/src/components/VGlobalAudioSection/VGlobalAudioSection.vue
Outdated
Show resolved
Hide resolved
frontend/src/components/VGlobalAudioSection/VGlobalAudioSection.vue
Outdated
Show resolved
Hide resolved
obulat
force-pushed
the
audio-control/global-audio
branch
2 times, most recently
from
31089f7 to
1e115d8
Compare
obulat
changed the title
fcoveram
reviewed
Oct 31, 2023
There was a problem hiding this comment.
I see the new border color, but I think it's set outside rather than inside because cover image and the white area on the right side have the same shade. Besides, the component corners look cut off.
Here is what I see
And here is the design
I tried deleting the branch and running the PR again, but I see the same.
obulat
force-pushed
the
audio-control/global-audio
branch
from
d4010cb to
8ff4069
Compare
|
@fcoveram, there were some child elements that were covering the border and making it look not The corners of the |
|
@obulat I'm seeing an issue where when I play an audio from the results list, then click on a single result, the audio stops playing. Is that intended? Otherwise this works as expected! |
The expected behavior was to stop the playback if you are opening a single result that is different from the audio that is currently playing. And if you start playing an audio and then go on to its single result page, the playback continues. |
fcoveram
reviewed
Nov 2, 2023
There was a problem hiding this comment.
The corners of the VAudioControl are the same as the global audio layout (4px) but it seems that they look a little bit more rounded so that there's a tiny gap between the button and the border.
The change is sublte. Let's keep the The VAudioControl corner as 4px is fine. Although, I did notice that effect in the cover image, especially in white ones. I think it inherits the brand yellow from the dot pattern. If that change requires too much time, feel free to dismiss it. It's also very subtle.
Regarding opacity in borders, I think the change does not work as expected and keeps the line outside the component area. I revisited Tailwind documentation and saw that "ring width" and "ring color" are used for these inner borders.
You can see how it works in Tailwind's homepage, in the Projects component example
obulat
force-pushed
the
audio-control/global-audio
branch
from
a859cf8 to
c91656a
Compare
AetherUnbound
approved these changes
Nov 2, 2023
obulat
force-pushed
the
audio-control/global-audio
branch
from
c91656a to
1d6cc45
Compare
|
Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR: @dhruvkb Excluding weekend1 days, this PR was ready for review 5 day(s) ago. PRs labelled with medium urgency are expected to be reviewed within 4 weekday(s)2. @obulat, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
obulat
force-pushed
the
audio-control/global-audio
branch
from
d817bdb to
60c94cc
Compare
dhruvkb
approved these changes
Nov 6, 2023
frontend/src/types/button.ts
Outdated
| @@ -36,5 +36,11 @@ export type ButtonSize = (typeof buttonSizes)[number] | |||
| export const buttonTypes = ["button", "submit", "reset"] as const | |||
| export type ButtonType = (typeof buttonTypes)[number] | |||
|
|
|||
| export const buttonConnections = ["start", "end", "none", "all"] as const | |||
| export const buttonConnections = [ | |||
There was a problem hiding this comment.
Technically the buttonConnections array should only contain 4 entries (one for each edge) and the connections prop on the VButton component should accept an array of connections. That would also remove the need for elements like "none" and "all".
It can be a complex change so I won't block on it, especially because this works.
There was a problem hiding this comment.
Having an array for buttonConnections would be much cleaner! I'll see if I can implement this.
github-actions
bot
added
🧱 stack: api
obulat
force-pushed
the
audio-control/global-audio
branch
from
51c24a2 to
6f1493d
Compare
Set border to dark-charcoal opacity-20, corner radius to 4px Signed-off-by: Olga Bulat <obulat@gmail.com>
Refactor Global layout to use CSS grid Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Refactor Global layout to use CSS grid Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
obulat
force-pushed
the
audio-control/global-audio
branch
from
d0207a2 to
cdb0a46
Compare
Signed-off-by: Olga Bulat <obulat@gmail.com>
fcoveram
approved these changes
Nov 7, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
Related to #2910 by @fcoveram
Description
This PR updates the Global audio player to match the design requirements in #2910.
It also fixes the problem we had with global audio player: it sometimes appeared on the home page, but didn't appear on the single result page. This was caused by the updates in the layouts, when I accidentally added the component into the wrong layout.
The old no longer used
VCloseButtoncomponent is also removed.Testing Instructions
Open the app on a mobile-width screen and search for something. When you click on Play button, you should be able to see the global player. You can seek, pause or play using it. If you click Play on a different audio, the audio should be updated. If you click on the audio title, the single result should open, and the global player should stay on the screen with the same audio playing.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin